Fixes #5001 and gossip panic when high concurrency #5078
Conversation
Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
This commit introduces the createResponse function in handler.go, which constructs a QueryResponse for range queries. It handles pagination and includes error handling for payload marshaling, ensuring proper cleanup of the query context in case of errors. Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
gossip/util/misc.go
Outdated
| var seed [32]byte | ||
| _, _ = crand.Read(seed[:]) | ||
| r = rand.New(rand.NewChaCha8(seed)) | ||
| r = rand.New(rand.NewSource(time.Now().UnixNano())) |
There was a problem hiding this comment.
why would we seed this with a deterministic and non-random seed?
what was before was better, in my opinion.
gossip/gossip/algo/pull.go
Outdated
| return n | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We shouldn't try 100 attempts, but more attempts, like 1000,000.
and if we still fail, then we should loop until we find a nonce that is not in use using the crypto rand.
There was a problem hiding this comment.
For the second part it can result in slow nonces if this happens right?
There was a problem hiding this comment.
after looking at the code myself, I think we can just use util.RandomUInt64 until it no longer exists in the outgoing nonces.
|
I don't understand how the gossip nonce generation is related to the panic, can you explain? |
gossip/util/misc.go
Outdated
| "math/rand/v2" | ||
| "math/rand" |
There was a problem hiding this comment.
Why did you change the library from v2 to the previous library?
There was a problem hiding this comment.
sorry, didn't mean to, changed back
Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
- Corrected comment capitalization for the HandleRegister function in handler.go. - Updated the import path for the math/rand package to math/rand/v2 in misc.go. These changes improve code readability and maintain consistency in import statements. Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
This commit introduces the import of the crypto/rand package in misc.go, which may be utilized for secure random number generation. This addition complements the existing math/rand/v2 import and prepares the code for future enhancements requiring cryptographic randomness. Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
This commit corrects the function name from `Intn` to `IntN` in the RandomInt function, ensuring consistency with the updated import path for the math/rand package. This change enhances code clarity and correctness. Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
gossip/util/misc.go
Outdated
| rMutex.Lock() | ||
| defer rMutex.Unlock() |
There was a problem hiding this comment.
why is the lock/unlock call only here?
There was a problem hiding this comment.
This is the panic that happens when there's a lot of concurrency:
panic: runtime error: index out of range [-1]
goroutine 3305 [running]:
math/rand.(*rngSource).Uint64(0xc014386101?)
/usr/local/go/src/math/rand/rng.go:249 +0x7b
math/rand.(*Rand).Uint64(0xc0180cf980?)
/usr/local/go/src/math/rand/rand.go:104 +0x22
github.com/hyperledger/fabric/gossip/util.RandomUInt64(...)
/gossip/util/misc.go:187
github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).newNONCE(0xc006953000)
/gossip/gossip/algo/pull.go:324 +0x25
github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).initiatePull(0xc006953000)
/gossip/gossip/algo/pull.go:179 +0x11b
github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter.func1()
/gossip/gossip/algo/pull.go:126 +0x27
created by github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter in goroutine 1
/gossip/gossip/algo/pull.go:120 +0x2d5
I suppose the problem is because we are accessing this function a lot of times, so adding a lock ensures that there are no concurrency issues
There was a problem hiding this comment.
-
Above r is used in 2 more places without lock. Why is lock not used there?
-
yes, this is a bug in the math/rand package. But after the release of v3.0.0 the package used 9b28d51 was changed, this change is not in the release yet. And your stacktrace is not using v2 code.
-
Need proof that you actually fixed the bug suddenly it was fixed by changing the package to version v2. You need a test to prove that your fixes are correct
/usr/local/go/src/math/rand/rng.go:249 +0x7b
math/rand.(*Rand).Uint64(0xc0180cf980?)
/usr/local/go/src/math/rand/rand.go:104 +0x22
There was a problem hiding this comment.
-
Above r is used in 2 more places without lock. Why is lock not used there?
-
yes, this is a bug in the math/rand package. But after the release of v3.0.0 the package used 9b28d51 was changed, this change is not in the release yet. And your stacktrace is not using v2 code.
-
Need proof that you actually fixed the bug suddenly it was fixed by changing the package to version v2. You need a test to prove that your fixes are correct
/usr/local/go/src/math/rand/rng.go:249 +0x7b
math/rand.(*Rand).Uint64(0xc0180cf980?)
/usr/local/go/src/math/rand/rand.go:104 +0x22
There was a problem hiding this comment.
This is the panic that happens when there's a lot of concurrency:
panic: runtime error: index out of range [-1]
goroutine 3305 [running]: math/rand.(*rngSource).Uint64(0xc014386101?) /usr/local/go/src/math/rand/rng.go:249 +0x7b math/rand.(*Rand).Uint64(0xc0180cf980?) /usr/local/go/src/math/rand/rand.go:104 +0x22 github.com/hyperledger/fabric/gossip/util.RandomUInt64(...) /gossip/util/misc.go:187 github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).newNONCE(0xc006953000) /gossip/gossip/algo/pull.go:324 +0x25 github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).initiatePull(0xc006953000) /gossip/gossip/algo/pull.go:179 +0x11b github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter.func1() /gossip/gossip/algo/pull.go:126 +0x27 created by github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter in goroutine 1 /gossip/gossip/algo/pull.go:120 +0x2d5
I suppose the problem is because we are accessing this function a lot of times, so adding a lock ensures that there are no concurrency issues
If there is a panic stemming from gossip then I fail to see how is the chaincode related to the problem.
We shouldn't fix two different problems in the same PR.
Can you move all the non gossip related code changes to a new PR?
There was a problem hiding this comment.
yeah, but you see the problem, right? gossip is invoking randomUint64, and it panicking,
There was a problem hiding this comment.
you are right, these are two different problems for two different panic errors
There was a problem hiding this comment.
panic calls a different code than the one you're correcting.
There was a problem hiding this comment.
So @pfi79 compiling the code in main should fix the issue right about gossip right?
There was a problem hiding this comment.
So @pfi79 compiling the code in main should fix the issue right about gossip right?
It's possible. After updating the package to v2 I stopped catching the panic error in tests.
gossip/gossip/algo/pull.go
Outdated
| @@ -319,10 +321,28 @@ func (engine *PullEngine) OnRes(items []string, nonce uint64) { | |||
| } | |||
|
|
|||
| func (engine *PullEngine) newNONCE() uint64 { | |||
| n := uint64(0) | |||
| engine.lock.Lock() | |||
There was a problem hiding this comment.
we don't need to lock at all. We only create nonces from here and we already lock there.
Just loop until engine.outgoingNONCES.Exists(n) is false, no need for attempts or anything.
Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
please update the pr description. there is not enough evidence that your change fixes the error when working with leveldb and please look at the unit test. after your changes they dropped exactly on leveldb. |
I just updated the description, I'm going to check the unit tests |
Signed-off-by: David VIEJO <dviejo@kungfusoftware.es>
|
Since you removed all gossip code from the PR, can't you just force push and get rid of the gossip code change commits? squash all changes together and force push so it will be more clear what's going on when this is merged, otherwise we'll merge a bunch of commits that cancel each other out. |
|
And I will tag @denyeart here, since the only code change in this PR is related to things he has more context about than me and pfi79. |
|
Thanks for the update @dviejokfs , although the PR has become confusing - 9 commits, a gossip title and bunch of gossip comments, but no gossip code, and 6 bullets in description related to chaincode and ledger, with no tests. It is unclear how everything relates. I suggest to reset - close this PR and open separate PRs for each individual improvement that can stand by itself. Each individual PR should have the smallest unit of code needed for the respective improvement, along with related unit test updates. The unit tests should demonstrate the problem being fixed (failing before the change, and passing after the change). Then the smaller individual PRs can each be judged on their own merit with clarity and focused discussion. Note that to keep the commits clean we often use the commit amend approach. This way you don't have to go through the process of squashing and then fixing up the commit/PR description. See details at https://hyperledger-fabric.readthedocs.io/en/latest/github/github.html#updating-a-pull-request. |
Type of change
Description
This pull request introduces several improvements in the
HandleGetStateByRangefunction within thecore/chaincode/handler.gofile and theGetStateRangeScanIteratorWithPaginationfunction within thecore/ledger/kvledger/txmgmt/statedb/stateleveldb/stateleveldb.gofile. The changes include:HandleGetStateByRangefunction to log errors and clean up any query contexts that may have been created.GetStateRangeScanIteratorWithPaginationto handle potential panics and return meaningful error messages.Additional details
These changes help improve the robustness of the code by handling potential panics and validating inputs, which can help prevent unexpected errors and improve the overall stability of the system.
Related issues
#5001